Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: Clarify when to construct a StringName ahead of time #79815

Merged
merged 1 commit into from
Feb 9, 2024
Merged

doc: Clarify when to construct a StringName ahead of time #79815

merged 1 commit into from
Feb 9, 2024

Conversation

idbrii
Copy link
Contributor

@idbrii idbrii commented Jul 23, 2023

I'm not certain my edit is 100% correct.

Clarify that manual StringName construction is an optimization for controlling when you pay the construction cost. Avoiding the word "optimization" so people don't use &"" everywhere by default.

Old text:

You will usually just pass a String to methods expecting a StringName and it will be automatically converted, but you may occasionally want to construct a StringName ahead of time with the StringName constructor or, in GDScript, the literal syntax &"example".

That immediately suggests the question: when and why?

I can only think of performance reasons:

  • when doing a lot of string comparisons
  • when StringName conversions occur every process or especially in a tight loop

I assume there's never a non-perf reason?

Uncertainty

I looked up the docs because the finite_state_machine demo uses &"" for Input.get_axis:

func get_input_direction():
	var input_direction = Vector2(
			Input.get_axis(&"move_left", &"move_right"),
			Input.get_axis(&"move_up", &"move_down")
	)
	return input_direction

I assume this is a good example of how to optimize with &"" and that using the literal &"" constructs the 4 StringNames at parse time and not per invocation of get_input_direction()?

If that's incorrect and this is the correct optimization:

var move_left = &"move_left"
var move_right = &"move_right"
var move_up = &"move_up"
var move_down = &"move_down"
func get_input_direction():
	var input_direction = Vector2(
			Input.get_axis(move_left, move_right),
			Input.get_axis(move_up, move_down)
	)
	return input_direction

Then a better final line would be:

Manual construction allows you to control when StringName conversions occur (moving them out of loops or hot code paths).

@idbrii idbrii requested a review from a team as a code owner July 23, 2023 09:53
@AThousandShips
Copy link
Member

AThousandShips commented Jul 23, 2023 •
edited
Loading

When using the &"" syntax the StringName is done by the compiler, so it doesn't matter if you move it out of a loop here

I think we should keep the original wording with "occasionally" as it should be done occasionally, depending on situation and not done too much

Also while I haven't verified it for built-in functions this optimization might be done for constant expressions anyway

Edit: It does indeed automatically convert constant strings to StringName for annotated functions:

func do_test(_var: StringName, _var2: String):
    pass

func test():
    do_test("abc", "def")

Yields:

Disassembling do_test(_var, _var2)
 0: line 2:     pass
 2: == END ==


Disassembling test()
 0: line 5:     do_test("abc", "def")
 2: call self.do_test(const(&"abc"), const("def"))
 10: assign stack(3) = false
 12: == END ==

This also happens with built-in functions such as those of Input

@AThousandShips AThousandShips added this to the 4.x milestone Jul 23, 2023
@idbrii
Copy link
Contributor Author

idbrii commented Jul 24, 2023

Ooh, great! It does even better than I expected!

Preference of either of these:

You will usually pass a [String] to methods expecting a [StringName] and it will be automatically converted (often at compile time), but in rare cases you can construct a [StringName] ahead of time with the [StringName] constructor or, in GDScript, the literal syntax [code]&"example"[/code]. Manual construction allows you to control when [StringName] conversions occur (moving them out of loops) or eliminate them (using the literal).

You will usually pass a [String] to methods expecting a [StringName] and it will be automatically converted (often at compile time), but you may occasionally want to construct a [StringName] ahead of time with the [StringName] constructor or, in GDScript, the literal syntax [code]&"example"[/code]. Manual construction allows you to control when [StringName] conversions occur (moving them out of loops) or eliminate them (using the literal).

I think the original use of "occasionally" was wordy, but understand the desire to discourage. Could also say "when the disassembler shows its necessary" with a link to how you disassembled but it doesn't look like there's a docs page for it.

@idbrii
Copy link
Contributor Author

idbrii commented Oct 8, 2023

I went with "in rare cases". Ready for review again.

@idbrii idbrii mentioned this pull request Oct 8, 2023
Closed
Mickeon
Mickeon reviewed Jan 26, 2024
View reviewed changes
Copy link
Contributor

@Mickeon Mickeon left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually looks really good. The "nudge" to use normal Strings with wording is great.

doc/classes/StringName.xml Outdated Show resolved Hide resolved
Mickeon
Mickeon approved these changes Feb 4, 2024
View reviewed changes
Copy link
Contributor

@Mickeon Mickeon left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to tweak it slightly but it's better than the original description. StringNames don't offer much advantages for most common users and they need to know.

doc/classes/StringName.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 5, 2024
@idbrii @akien-mga
doc: Clarify when to construct a StringName ahead of time
9cbc402 
Fix docs don't give justification for manual construction.

Clarify how to apply manual StringName construction as an
optimization and that "string intern" means "work at parse time".

There are several godot-demo-projects (including 3d/platformer) that
incorrectly use StringName literals (they use & literals instead of just
passing strings), so clearly this is confusing.

AThousandShips did a disassembly test to prove it automatically converts
constant strings to StringName for annotated functions:

  func do_test(_var: StringName, _var2: String):
  pass

  func test():
  do_test("abc", "def")

Yields:

  Disassembling do_test(_var, _var2)
   0: line 2:     pass
   2: == END ==

  Disassembling test()
   0: line 5:     do_test("abc", "def")
   2: call self.do_test(const(&"abc"), const("def"))
   10: assign stack(3) = false
   12: == END ==

It also happens with built-in functions such as those of Input.
@akien-mga akien-mga merged commit 158df3e into godotengine:master Feb 9, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@idbrii idbrii deleted the patch-2 branch February 12, 2024 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@Mickeon Mickeon Mickeon approved these changes

@akien-mga akien-mga akien-mga left review comments

Assignees
No one assigned
Labels
documentation enhancement
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
4 participants
@idbrii @AThousandShips @akien-mga @Mickeon